fix: return 404 on undefined value - #1034
Conversation
WalkthroughA guard clause was introduced at the beginning of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware (prepareEntity)
participant Next
Client->>Middleware (prepareEntity): Request with issueEntities
alt issueEntities missing or empty
Middleware (prepareEntity)->>Next: next(MiddlewareError("No issues for entity", 404))
else issueEntities present
Middleware (prepareEntity)->>Middleware (prepareEntity): Continue with normal processing
Middleware (prepareEntity)->>Next: next()
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/middleware/entityIssueDetails.middleware.js (1)
91-93: Good addition of a guard clause to prevent 500 errors.This guard clause effectively prevents accessing an undefined array element when
issueEntitiesis empty or missing, directly addressing the bug causing the 500 error. Returning a 404 with a clear error message is appropriate for this case.However, the condition could be slightly simplified:
- if (!issueEntities || (issueEntities && issueEntities.length === 0)) { + if (!issueEntities || issueEntities.length === 0) { return next(new MiddlewareError('No issues for entity', 404)) }This works because if
issueEntitiesis falsy, the right side won't evaluate due to short-circuit evaluation.test/unit/middleware/entityIssueDetails.middleware.test.js (1)
47-67: Test name and implementation mismatch.The test names in lines 47 and 58 mention checking
req.issues, but the implementation insetRecordCount(line 70 in the middleware file) actually checksreq.issueEntities. This could lead to confusion.- it('sets the record count to 0 if req.issues is undefined', () => { + it('sets the record count to 0 if req.issueEntities is undefined', () => {And similarly:
- it('sets the record count to 0 if req.issues is null', () => { - const req = { issues: null } + it('sets the record count to 0 if req.issueEntities is null', () => { + const req = { issueEntities: null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/middleware/entityIssueDetails.middleware.js(1 hunks)test/unit/middleware/entityIssueDetails.middleware.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/unit/middleware/entityIssueDetails.middleware.test.js (1)
Learnt from: rosado
PR: digital-land/submit#657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
🧬 Code Graph Analysis (1)
test/unit/middleware/entityIssueDetails.middleware.test.js (2)
src/middleware/entityIssueDetails.middleware.js (3)
req(88-88)req(155-155)prepareEntity(87-144)src/utils/errors.js (1)
MiddlewareError(17-34)
🔇 Additional comments (3)
src/middleware/entityIssueDetails.middleware.js (1)
87-144:Details
❓ Verification inconclusive
Consider relationship between prepareEntity's new guard clause and show404ifNoIssues middleware.
Both
prepareEntityandshow404ifNoIssuesnow handle the case of missing issues, but in slightly different ways and at different points in the middleware chain. This is likely intentional to provide different error messages based on context, but it's worth ensuring that having both checks is the desired behavior.Looking at the middleware chain order in lines 174-195,
show404ifNoIssuesruns beforeprepareEntity, suggesting the new guard clause might never be reached in normal operation. Could you verify this is the intended behavior?
🏁 Script executed:
#!/bin/bash # Check the relative position of both middlewares in the chain echo "Middleware chain position:" grep -n "show404ifNoIssues\|prepareEntity" src/middleware/entityIssueDetails.middleware.jsLength of output: 312
🏁 Script executed:
#!/bin/bash # Display the implementation of show404ifNoIssues for comparison rg -n "export const show404ifNoIssues" -A10 src/middleware/entityIssueDetails.middleware.jsLength of output: 373
Review redundancy between show404ifNoIssues and prepareEntity
The
show404ifNoIssuesmiddleware (lines 154–162) already returns a 404 whenrecordCount === 0beforeprepareEntityever runs (line 191). Meanwhile,prepareEntityhas its own guard (lines 87–91) that checks for missing or emptyissueEntities, which corresponds to the same condition and will never be reached in normal operation.Please confirm whether this duplicate check is intentional or whether the guard in
prepareEntityshould be removed or changed (for example, to handle out‑of‑rangepageNumbercases instead).• src/middleware/entityIssueDetails.middleware.js
– prepareEntity guard at lines 87–91
– show404ifNoIssues implementation at lines 154–162test/unit/middleware/entityIssueDetails.middleware.test.js (2)
71-89: Good refactoring using template objects and deep cloning.Using template objects (
reqTemplateandresTemplate) withstructuredClonein thebeforeEachhook is a good practice that ensures proper test isolation. This prevents test cases from mutating shared objects and affecting each other's results.
162-169: Well-written test for the new guard clause.This test properly verifies the new guard clause behaviour, ensuring that when
issueEntitiesis empty:
req.entryremains undefinednextis called with aMiddlewareErrorwith the correct message and status codeThis test case effectively covers the bug fix described in the PR objectives.
2 participants
What type of PR is this? (check all applicable)
Description
An udefined value causes a 500, we should return a 404 instead.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Going to this URL should return a 404, and not a 500
Added/updated tests?
QA sign off
Summary by CodeRabbit